Add scipy.linalg.lu() decomposition support#2787
Add scipy.linalg.lu() decomposition support#2787abagusetty wants to merge 14 commits intoIntelPython:masterfrom
scipy.linalg.lu() decomposition support#2787Conversation
abagusetty
requested review from
antonwolfy,
ndgrigorian and
vlad-perevezentsev
as code owners
|
Can one of the admins verify this patch? |
antonwolfy
left a comment
antonwolfy
left a comment
There was a problem hiding this comment.
@abagusetty, thank you for the contribution. I posted few comments below
| from ._decomp_lu import lu, lu_factor, lu_solve | ||
|
|
||
| __all__ = [ | ||
| "lu", |
There was a problem hiding this comment.
Could you please state the change in the CHANGELOG.md?
dpnp/scipy/linalg/_utils.py
Outdated
| usm_type=a_usm_type, | ||
| sycl_queue=a_sycl_queue, | ||
| ) | ||
| up = dpnp.array(a, dtype=res_type) |
There was a problem hiding this comment.
looks like scipy might return input array a if dtype matches expected and overwrite_a is True:
| up = dpnp.array(a, dtype=res_type) | |
| up = dpnp.astype(a, dtype=res_type, copy=not overwrite_a) |
dpnp/scipy/linalg/_utils.py
Outdated
| inv_perm = dpnp.zeros( | ||
| (*batch_shape, 1), | ||
| dtype=dpnp.int64, | ||
| usm_type=a_usm_type, | ||
| sycl_queue=a_sycl_queue, | ||
| ) |
There was a problem hiding this comment.
we can use zeros_like here also (that will simplify the code):
| inv_perm = dpnp.zeros( | |
| (*batch_shape, 1), | |
| dtype=dpnp.int64, | |
| usm_type=a_usm_type, | |
| sycl_queue=a_sycl_queue, | |
| ) | |
| inv_perm = dpnp.zeros_like( | |
| a, | |
| shape=(*batch_shape, 1), | |
| dtype=dpnp.int64, | |
| ) |
dpnp/scipy/linalg/_utils.py
Outdated
| ) | ||
|
|
||
| # ---- Fast path: empty arrays ---- | ||
| elif a.size == 0: |
There was a problem hiding this comment.
we can use empty_like for below calls
dpnp/scipy/linalg/_utils.py
Outdated
|
|
||
| # The permutation matrix P uses a real dtype (SciPy convention): | ||
| # P only contains 0s and 1s, so complex storage would be wasteful. | ||
| real_type = _get_real_dtype(res_type) |
There was a problem hiding this comment.
Can we reuse _real_type here instead?
| real_type = _get_real_dtype(res_type) | |
| real_type = _real_type(res_type) |
| usm_type=a_usm_type, | ||
| sycl_queue=a_sycl_queue, | ||
| ) | ||
|
|
There was a problem hiding this comment.
Would it be better (and faster) just to return here and in scalar path above to avoid possible below calls of _apply_permutation_to_rows?
There was a problem hiding this comment.
I agree, the empty path previously used dpnp.empty_like for perm_matrix to skip the gather entirely, but since both eye_m and inv_perm are empty arrays, _apply_permutation_to_rows on them is a no-op. Also to deal with multiple returns, I have created a helper method _assemble_lu_output to reduce the clutter and please pylint warning
| a[1, 0, 0] = dpnp.nan | ||
| assert_raises(ValueError, dpnp.scipy.linalg.lu, a, check_finite=True) | ||
|
|
||
|
|
There was a problem hiding this comment.
Please also enable muted corresponded tests in dpnp/tests/third_party/cupyx/scipy_tests/linalg_tests/test_decomp_lu.py
Co-authored-by: Anton <100830759+antonwolfy@users.noreply.github.com>
Co-authored-by: Anton <100830759+antonwolfy@users.noreply.github.com>
|
@antonwolfy Thanks for taking time. All the suggestions were addressed. Please let me know |
3 participants
This PR adds
dpnp.scipy.linalg.lu()with support for all three output modes: default(P, L, U),permute_l=True (PL, U), andp_indices=True(p, L, U), including batched inputs.Fixes: #2786